Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Improve communication of serialised Realm objects between device and plugin #123

Merged
merged 21 commits into from
Jan 18, 2023
Merged

Improve communication of serialised Realm objects between device and plugin #123

merged 21 commits into from
Jan 18, 2023

Conversation

gagik
Copy link
Contributor

@gagik gagik commented Jan 12, 2023 •
edited
Loading

This PR...

TODO:

  • Empty collections being sent over not handled correctly
  • Embedded object modification not handled correctly
  • Test modification of different types
  • Test expanding data

@cla-bot cla-bot bot added the cla: yes label Jan 12, 2023
@gagik gagik changed the base branch from main to gagik/typescriptrefactoring January 12, 2023 13:05
@gagik gagik mentioned this pull request Jan 12, 2023
Closed
This was linked to issues Jan 12, 2023
Flattening of linked objects #85
Closed
Special property names #86
Closed
Object Key in data inspector #93
Closed
Circular references #96
Closed
Embedded Object Support #113
Closed
@gagik gagik force-pushed the gagik/typescriptrefactoring branch from 3f036e3 to fdd81eb Compare January 12, 2023 14:29
@gagik gagik mentioned this pull request Jan 12, 2023
Merged
@gagik gagik marked this pull request as ready for review January 16, 2023 16:50
takameyer
takameyer approved these changes Jan 17, 2023
View reviewed changes
Copy link
Contributor

@takameyer takameyer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Really happy about all the work you've put into this. Looks like the flipper plugin is now useable!
My only suggestion is to consider breaking up "common" and "shared" and getting some of those types closer to the component using them. But this could be done later, just wanted to provide something to think about.
LGTM 🚢

flipper-plugin-realm/src/components/objectManipulation/types/ObjectInput.tsx
}
// TODO: Fetch object with getObject to display primary key instead of objectKey
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Did you want to address this in a separate PR, or is this doable now?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was thinking of separate PR. although I just thought of a way to make this simpler, i.e. just send over primaryKey if it exists along with objectType / objectKey for references, so I might just do this. I generally avoid leaving TODOs like this but because of the state of the code right now this seemed like the quickest way to leave some bits for future planning.

Base automatically changed from gagik/typescriptrefactoring to main January 18, 2023 12:28
@gagik gagik merged commit 7da4dec into main Jan 18, 2023
@gagik gagik linked an issue Jan 18, 2023 that may be closed by this pull request
Implement a new approach to communicating serialised Realm objects between device and plugin #121
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@takameyer takameyer takameyer approved these changes

Assignees
No one assigned
Labels
cla: yes
Projects
None yet
Milestone
No milestone
2 participants
@gagik @takameyer